Skip to content

add support for stop_signal to compose file#2508

Merged
aanand merged 2 commits into
docker:masterfrom
jstewmon:stop_signal
Jan 14, 2016
Merged

add support for stop_signal to compose file#2508
aanand merged 2 commits into
docker:masterfrom
jstewmon:stop_signal

Conversation

@jstewmon
Copy link
Copy Markdown

@jstewmon jstewmon commented Dec 4, 2015

Based on #2232
Depends on docker/docker-py#868
Fixes #576

@thaJeztah
Copy link
Copy Markdown
Member

Looks like something went wrong here, because this PR contains unrelated commits 😢

@thaJeztah
Copy link
Copy Markdown
Member

(I see that docker/docker-py#868 was merged btw)

@jstewmon
Copy link
Copy Markdown
Author

@thaJeztah this PR is intentionally based on #2232 (the "unrelated commits") because stop signal support was introduced in version 1.21 of the remote API.

@dnephin advised basing this PR on #2232 and bumping DEFAULT_API_VERSION to 1.21.

When #2232 is merged, I will rebase on master.

@thaJeztah
Copy link
Copy Markdown
Member

@jstewmon ahh, I see now, sorry; usually "unrelated" commits are a result of a bad rebase. Sorry for the noise 😄

@jstewmon
Copy link
Copy Markdown
Author

jstewmon commented Jan 7, 2016

I've rebased this PR on master, now that v1.21 support has been merged.

Comment thread tests/acceptance/cli_test.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be removed now that 1.21 is the default

@dnephin dnephin added this to the 1.6.0 milestone Jan 7, 2016
@dnephin
Copy link
Copy Markdown

dnephin commented Jan 7, 2016

I think this looks good, just a couple small changes to the test

@jstewmon
Copy link
Copy Markdown
Author

jstewmon commented Jan 7, 2016

@dnephin , Thanks for the quick review. I've removed those extraneous lines.

@dnephin
Copy link
Copy Markdown

dnephin commented Jan 12, 2016

This needs another rebase. I believe you'll need to add this to the v2 schema as well.

Signed-off-by: Jonathan Stewmon <jstewmon@rmn.com>
@jstewmon
Copy link
Copy Markdown
Author

Rebased and updated both service schema versions.

@dnephin
Copy link
Copy Markdown

dnephin commented Jan 13, 2016

There's a test failure after the rebase

@jstewmon
Copy link
Copy Markdown
Author

The tests fail because the docker-py version being used in CI is 1.6, and this PR depends on docker/docker-py#868 which is slated for docker-py 1.7.

The tests pass against the docker-py master branch.

I looked through the boilerplate code for tests to see if there was a way of skipping the test based on the version of module, but didn't see one.

Is there a standard way of handling yet-to-be-released dependencies on docker-py?

@aanand
Copy link
Copy Markdown

aanand commented Jan 13, 2016

Use a git URL in requirements.txt, like this:

git+https://github.com/docker/docker-py.git@9deffc45a10bf0d03907002a0b3fa6b63c7ff817#egg=docker-py

Signed-off-by: Jonathan Stewmon <jstewmon@rmn.com>
@dnephin
Copy link
Copy Markdown

dnephin commented Jan 14, 2016

LGTM!

@aanand
Copy link
Copy Markdown

aanand commented Jan 14, 2016

LGTM

aanand added a commit that referenced this pull request Jan 14, 2016
add support for stop_signal to compose file
@aanand aanand merged commit 7733600 into docker:master Jan 14, 2016
@jstewmon jstewmon deleted the stop_signal branch April 4, 2017 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants